Fix Worksheet Passwords #2197
Merged
Fix Worksheet Passwords #2197
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix for issue PHPOffice#1897. The existing hashing code seems to work correctly almost all the time, but there are exceptions. It is replaced by an exact implementation of the spec, including a link to the spec in the comments. Cases known to fail are added to the unit test suite. The spec expects the string to be at most 255 bytes (yes, bytes not characters). The program had permitted any length; it will now throw an exception when the maximum length is exceeded. Xls does not support any hashing algorithm except basic. The Xls writer had, nevertheless, accepted the results of any of the other possible algorithms. This leads to (a) a worksheet that can't be unprotected, and (b) deprecation notices during the write (because it is using hexdec, which expects only hex characters, and the other algorithms generate non-hex characters). I have changed Xls writer to ignore passwords generated by other algorithms. An alternative would be to have the password hasher generate both an algorithmic password (for use by Xlsx) and a basic password (for use by Xls); I think that is too complex a solution, but can look into it if you think it worthwhile. I do not see any current support for Worksheet passwords in ODS Reader or Writer. I did not add support in this PR. I added a new test to confirm the password for reading a spreadsheet is consistent with the one used for writing it. As you can see from the comments for the new test, it had an unusual problem with a somewhat unusual solution.
|
I found the test (SettingsTest) which created the unusual problem for my new test. Expect a new commit/push to this PR later today. |
SettingsTest was changing the global LibXMLLoaderOptions without restoring the original. This caused problems for one of my new tests.
7 tests that needed to invoke Settings::setLibXmlLoaderOptions no longer need to do so.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for issue #1897.
The existing hashing code seems to work correctly almost all the time, but there are exceptions. It is replaced by an exact implementation of the spec, including a link to the spec in the comments. Cases known to fail are added to the unit test suite.
The spec expects the string to be at most 255 bytes (yes, bytes not characters). The program had permitted any length; it will now throw an exception when the maximum length is exceeded.
Xls does not support any hashing algorithm except basic. The Xls writer had, nevertheless, accepted the results of any of the other possible algorithms. This leads to (a) a worksheet that can't be unprotected, and (b) deprecation notices during the write (because it is using hexdec, which expects only hex characters, and the other algorithms generate non-hex characters). I have changed Xls writer to ignore passwords generated by other algorithms. An alternative would be to have the password hasher generate both an algorithmic password (for use by Xlsx) and a basic password (for use by Xls); I think that is too complex a solution, but can look into it if you think it worthwhile. The documentation mentions that the other algorithms are usable only for Xlsx, so I don't think a doc change is required.
I do not see any current support for Worksheet passwords in ODS Reader or Writer. I did not add support in this PR.
I added a new test to confirm the password for reading a spreadsheet is consistent with the one used for writing it. As you can see from the comments for the new test, it had an unusual problem with a somewhat unusual solution.
This is:
Checklist:
Why this change is needed?